Conversation
|
/ok to test eb6ddb9 |
Phlip79
changed the title
|
@greptile Please review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request reassigns code ownership in CODEOWNERS to more specialized teams by removing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/multi-approval-bot.yml:
- Around line 83-93: The current APPROVERS extraction uses all historical
APPROVED reviews and can count stale approvals; change the gh api/jq pipeline
that sets APPROVERS so it groups reviews by .user.login and selects each
reviewer’s latest review (e.g., sort_by(.submitted_at) | last) and then filters
those latest reviews for state == "APPROVED"; update the variable referenced as
APPROVERS and keep the subsequent loop over MEMBERS and the grep check unchanged
so only reviewers whose latest review is APPROVED are considered.
- Around line 111-121: The APPROVERS extraction can include stale approvals
because it doesn't reduce multiple reviews per user to their latest state;
update the APPROVERS assignment (the gh api call that produces APPROVERS) to
sort reviews by submitted_at, group by .user.login, take the last review per
user, then select those with state == "APPROVED" and extract their login; this
ensures APPROVERS only contains users whose most recent review is APPROVED when
you later compare against MEMBERS in the for-loop.
🧹 Nitpick comments (1)
.github/workflows/multi-approval-bot.yml (1)
53-68: Silent failure handling may mask configuration issues.The
2>/dev/null || truepattern ensures the workflow doesn't fail if review requests can't be made. While this is reasonable for a non-critical operation (requesting reviews), it could mask issues like:
- Insufficient PAT permissions for team review requests
- Team names that don't exist or have changed
Consider adding a warning log when the request fails to aid in debugging:
💡 Suggested improvement for better observability
# Request core-nemo for core changes - gh api repos/${{ github.repository }}/pulls/$PR_NUMBER/requested_reviewers \ - --method POST -f team_reviewers[]="core-nemo" 2>/dev/null || true + gh api repos/${{ github.repository }}/pulls/$PR_NUMBER/requested_reviewers \ + --method POST -f team_reviewers[]="core-nemo" 2>/dev/null || echo "Warning: Could not request review from core-nemo" # Request core-adlr unless complexity:low if [ "$IS_LOW_COMPLEXITY" != "true" ]; then gh api repos/${{ github.repository }}/pulls/$PR_NUMBER/requested_reviewers \ - --method POST -f team_reviewers[]="core-adlr" 2>/dev/null || true + --method POST -f team_reviewers[]="core-adlr" 2>/dev/null || echo "Warning: Could not request review from core-adlr" fi
| # Get PR approvers | ||
| APPROVERS=$(gh api repos/${{ github.repository }}/pulls/$PR_NUMBER/reviews \ | ||
| --jq '[.[] | select(.state == "APPROVED")] | [.[].user.login] | unique | .[]') | ||
|
|
||
| # Check if any team member approved | ||
| for member in $MEMBERS; do | ||
| if echo "$APPROVERS" | grep -q "^${member}$"; then | ||
| echo "✅ core-nemo approval found from: $member" | ||
| exit 0 | ||
| fi | ||
| done |
There was a problem hiding this comment.
Potential issue: Stale approvals may be counted.
The current logic fetches all reviews with state APPROVED and checks if any team member is in that list. However, GitHub's reviews API returns all historical reviews. If a reviewer first approves, then later submits a "Request Changes" review, the old approval will still be counted.
Consider filtering to only count the latest review state per reviewer:
🐛 Proposed fix to consider only the latest review per reviewer
# Get PR approvers
- APPROVERS=$(gh api repos/${{ github.repository }}/pulls/$PR_NUMBER/reviews \
- --jq '[.[] | select(.state == "APPROVED")] | [.[].user.login] | unique | .[]')
+ # Get the latest review state per reviewer, then filter for APPROVED
+ APPROVERS=$(gh api repos/${{ github.repository }}/pulls/$PR_NUMBER/reviews \
+ --jq 'group_by(.user.login) | map(max_by(.submitted_at)) | [.[] | select(.state == "APPROVED")] | .[].user.login')🤖 Prompt for AI Agents
In @.github/workflows/multi-approval-bot.yml around lines 83 - 93, The current
APPROVERS extraction uses all historical APPROVED reviews and can count stale
approvals; change the gh api/jq pipeline that sets APPROVERS so it groups
reviews by .user.login and selects each reviewer’s latest review (e.g.,
sort_by(.submitted_at) | last) and then filters those latest reviews for state
== "APPROVED"; update the variable referenced as APPROVERS and keep the
subsequent loop over MEMBERS and the grep check unchanged so only reviewers
whose latest review is APPROVED are considered.
| # Get PR approvers | ||
| APPROVERS=$(gh api repos/${{ github.repository }}/pulls/$PR_NUMBER/reviews \ | ||
| --jq '[.[] | select(.state == "APPROVED")] | [.[].user.login] | unique | .[]') | ||
|
|
||
| # Check if any team member approved | ||
| for member in $MEMBERS; do | ||
| if echo "$APPROVERS" | grep -q "^${member}$"; then | ||
| echo "✅ core-adlr approval found from: $member" | ||
| exit 0 | ||
| fi | ||
| done |
There was a problem hiding this comment.
Same stale approval issue applies here.
This step has the same potential issue as the core-nemo check where historical approvals may be counted even if the reviewer later changed their review state. The same fix should be applied here.
🐛 Proposed fix for core-adlr approval check
# Get PR approvers
- APPROVERS=$(gh api repos/${{ github.repository }}/pulls/$PR_NUMBER/reviews \
- --jq '[.[] | select(.state == "APPROVED")] | [.[].user.login] | unique | .[]')
+ # Get the latest review state per reviewer, then filter for APPROVED
+ APPROVERS=$(gh api repos/${{ github.repository }}/pulls/$PR_NUMBER/reviews \
+ --jq 'group_by(.user.login) | map(max_by(.submitted_at)) | [.[] | select(.state == "APPROVED")] | .[].user.login')🤖 Prompt for AI Agents
In @.github/workflows/multi-approval-bot.yml around lines 111 - 121, The
APPROVERS extraction can include stale approvals because it doesn't reduce
multiple reviews per user to their latest state; update the APPROVERS assignment
(the gh api call that produces APPROVERS) to sort reviews by submitted_at, group
by .user.login, take the last review per user, then select those with state ==
"APPROVED" and extract their login; this ensures APPROVERS only contains users
whose most recent review is APPROVED when you later compare against MEMBERS in
the for-loop.
2 participants
What does this PR do ?
This PR changes a few things for review logic:
core-nemoandcore-adlras code owners for all code that has an expert group.core-nemoas a required reviewer in a separate step to core and training codecore-adlras a required reviewer in a separate step to core and training code if the PR is not labeled withcomplexity: lowNote: if
core-adlris still a code owner and the PR has labelcomplexity: low,core-adlrwill still be required to review.Contribution process
flowchart LR A[Pre-checks] --> B[PR Tests] subgraph Code Review/Approval C1[Expert Review] --> C2[Final Review] end B --> C1 C2 --> D[Merge]Pre-checks
Core 0.8)Code review
The following process is enforced via the CODEOWNERS file for changes into
megatron/core. For changes outside ofmegatron/core, it is up to the PR author whether or not to tag the Final Reviewer team.For MRs into `main` branch
Feel free to message or comment the @mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
(Step 1): Add PR label
Expert Review(Step 2): Collect the expert reviewers reviews
Expert Reviewlabel when your PR is ready for review.Final Review might get declined if these requirements are not fulfilled.
(Step 3): Final Review
Final Reviewlabel(Optional Step 4): Cherry-pick into release branch
If this PR also needs to be merged into
core_r*release branches, after this PR has been merged, selectCherry-pickto open a new PR into the release branch.For MRs into `dev` branch
The proposed review process for `dev` branch is under active discussion.MRs are mergable after one approval by either
eharper@nvidia.comorzijiey@nvidia.com.Merging your PR
Any member of core-adlr and
core-nemowill be able to merge your PR.Summary by CodeRabbit